fix(webapp): harden RouteErrorDisplay against null error.data - #4423
fix(webapp): harden RouteErrorDisplay against null error.data#4423Ayush7614 wants to merge 1 commit into
Conversation
Remix route errors can have null/empty data; reading .message threw inside the error boundary and blanked the page. Fall back safely.
|
|
Hi @Ayush7614, thanks for your interest in contributing! This project requires that pull request authors are vouched, and you are not in the list of vouched users. This PR will be closed automatically. See https://github.com/triggerdotdev/trigger.dev/blob/main/CONTRIBUTING.md for more details. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
WalkthroughAdds ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| it("falls back when data is null", () => { | ||
| expect(getRouteErrorMessage(500, "Internal Server Error", null)).toBe( | ||
| "Something went wrong on our end. Please try again later." | ||
| ); | ||
| }); | ||
|
|
||
| it("falls back when data is undefined", () => { | ||
| expect(getRouteErrorMessage(404, "Not Found", undefined)).toBe( | ||
| "The page you're looking for doesn't exist." | ||
| ); | ||
| }); | ||
|
|
||
| it("uses a string data body", () => { | ||
| expect(getRouteErrorMessage(400, "Bad Request", "Invalid payload")).toBe("Invalid payload"); | ||
| }); | ||
|
|
||
| it("falls back when message is missing or empty", () => { | ||
| expect(getRouteErrorMessage(403, "Forbidden", {})).toBe( | ||
| "You don't have permission to access this resource." | ||
| ); | ||
| expect(getRouteErrorMessage(403, "Forbidden", { message: "" })).toBe( | ||
| "You don't have permission to access this resource." | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🔴 New error-message tests fail because fallback returns the status text, not the generic message
The fallback message is built from the shared helper (friendlyErrorDisplay(status, statusText).message at apps/webapp/app/utils/httpErrors.ts:50), which returns the provided status text whenever one is given rather than the generic default the new tests expect, so the added test suite fails.
Impact: The project's test run breaks, blocking the change from passing CI.
Mechanism: statusText short-circuits the default message
friendlyErrorDisplay returns statusText ?? <default> for every case (apps/webapp/app/utils/httpErrors.ts:5-43). Every new test passes a truthy statusText (e.g. "Internal Server Error", "Not Found", "Forbidden"), so friendlyErrorDisplay(...).message evaluates to that status text, never the generic default.
Concretely:
getRouteErrorMessage(500, "Internal Server Error", null)returns"Internal Server Error", but the test atapps/webapp/app/utils/httpErrors.test.ts:10-12expects"Something went wrong on our end. Please try again later."getRouteErrorMessage(404, "Not Found", undefined)returns"Not Found", butapps/webapp/app/utils/httpErrors.test.ts:16-18expects"The page you're looking for doesn't exist."getRouteErrorMessage(403, "Forbidden", {})and(403, "Forbidden", { message: "" })return"Forbidden", butapps/webapp/app/utils/httpErrors.test.ts:26-31expects"You don't have permission to access this resource."
Either the implementation must resolve the generic default when data is missing (e.g. call friendlyErrorDisplay(status) without statusText), or the test expectations must be corrected to the status-text values.
Prompt for agents
The new tests in apps/webapp/app/utils/httpErrors.test.ts will fail because getRouteErrorMessage computes its fallback as friendlyErrorDisplay(status, statusText).message, and friendlyErrorDisplay returns `statusText ?? <default>` — so whenever a truthy statusText is supplied (as every test does), the fallback equals the status text, not the generic default the assertions expect. Decide the intended behavior: (1) if callers should see the friendly generic message when error.data is missing, change getRouteErrorMessage's fallback to friendlyErrorDisplay(status).message (omit statusText) so the default is used, and note this also changes RouteErrorDisplay's runtime output; or (2) if showing the status text is intended (matching the prior behavior of error.data.message ?? friendlyErrorDisplay(...).message), update the test expectations in httpErrors.test.ts to expect the passed statusText strings (e.g. 'Internal Server Error', 'Not Found', 'Forbidden'). Ensure the tests and implementation agree.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
RouteErrorDisplayreaderror.data.messagewithout a null check. Remix route errors can havedata: null, a string, or an empty object, which crashed the error boundary and blanked the page. AddgetRouteErrorMessagewith safe fallbacks + unit tests.✅ Checklist
Testing
apps/webapp/app/utils/httpErrors.test.tscovering null/undefined/string/{ message }casesRouteErrorDisplayusesgetRouteErrorMessageinstead of directerror.data.messageaccessfriendlyErrorDisplaywhen data is missingChangelog
Dashboard error pages no longer crash when a route error has no data payload.